-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CI after Rust 1.83 #246
Conversation
.github/workflows/ci.yml
Outdated
OPTIONS: ${{ matrix.platform.options }} | ||
# Disable doc tests on Rust 1.83 | ||
OPTIONS: ${{ matrix.platform.options }} ${{ matrix.rust_version == 'stable' && '--lib' || '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we link an issue or comment describing this problem in more detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I linked to rust-lang/rust#132203
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing your due diligence to look up the actual upstream issue. I'll comment there later because it doesn't seem to be explicitly mentioned that this "regression" has trickled into 1.83 - i.e. they didn't fix / back it out before the release.
At the same time I hate to call this a regression, it's a good change. As outlined in #127 (comment) already, I feel like having the path relative to README.md
(i.e. the span of that file) is much more sensible than having it relative to whichever random file (or files, it could be multiple in different locations) is/are including README.md
.
But that requires a breaking change...
EDIT: upstream Rust issues and pull requests are incredibly poorly documented. They all reference each other, but none of them have an accurate summary describing what they're observing or changing, requiring one to follow links all the way to the beginning, then unwind again to understand what is going on:
- Span in
rustdoc
errors is incorrectly reported forinclude_str!()
: rustdoc reports issues againstlib.rs
rather than aninclude_str!
'd file rust-lang/rust#130470 - Fix for that: rustdoc: use the correct span for doctests rust-lang/rust#130582
- That fix regresses recursive imports: regression: change in paths/CWD for rustdoc tests rust-lang/rust#132203
- Fixing regression by making it an edition 2024 change: rustdoc: make doctest span tweak a 2024 edition change rust-lang/rust#132210
- Decision to change such behaviour must have a tracking issue etc: Tracking issue for Rust 2024: Fix doctest
include
paths rust-lang/rust#132230
(Notice my frustration again: this spends more words explaining what a tracking issue is, than describing what it is tracking beyond "fixing the path used for include in rustdoc doctests")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoying that this issue got into Rust 1.83, but everything here looks good.
The path changes that @MarijnS95 outlined here seem to have been fixed in beta and in more recent nightlies, but did slip into the 1.83 release. So I've disabled doc tests on 1.83 in CI for now.